Skip to content

refactor: move category types and validation logic to shared package#2877

Closed
cqnykamp wants to merge 5 commits intoDoenet:mainfrom
cqnykamp:refactor-move-browsable-validation
Closed

refactor: move category types and validation logic to shared package#2877
cqnykamp wants to merge 5 commits intoDoenet:mainfrom
cqnykamp:refactor-move-browsable-validation

Conversation

@cqnykamp
Copy link
Copy Markdown
Contributor

Summary

Move category-related types and logic to the shared package to reduce duplication and enable reuse across the platform.

Changes

  • Types moved to packages/shared/src/types/categories.ts

    • CategoryGroup - represents a group of related categories with constraints
    • Category - represents an individual category
  • Logic moved to packages/shared/src/logic/browsable.ts

    • isActivityFullyCategorized() - validates that all required category groups have at least one category selected
  • Imports updated across app and api workspaces to use @doenet-tools/shared

Files Modified

  • 18 files updated with new imports
  • 2 new files in shared package
  • 13 lines removed from duplicate type definitions

The shared package builds successfully.

- Move CategoryGroup and Category types to packages/shared/src/types/categories.ts
- Move isActivityFullyCategorized() logic to packages/shared/src/logic/browsable.ts
- Update all imports across app and api workspaces to use the shared package
- Export new types and logic from packages/shared/src/index.ts

This allows category-related code to be reused across the platform without duplication.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR centralizes category-related domain types and the “fully categorized” validation helper into @doenet-tools/shared so both the app and API can reuse the same definitions and avoid duplicated logic.

Changes:

  • Added shared Category / CategoryGroup types and exported them from the shared package entrypoint.
  • Moved isActivityFullyCategorized() into shared logic and re-exported it.
  • Updated app + API imports to consume the shared types/function and removed local duplicates.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/shared/src/types/categories.ts New shared category type definitions.
packages/shared/src/logic/browsable.ts New shared isActivityFullyCategorized() implementation.
packages/shared/src/index.ts Re-export shared category types and browsable logic from package root.
apps/app/src/widgets/editor/EditCategories.tsx Switch category type imports to @doenet-tools/shared.
apps/app/src/widgets/editor/EditCategories.cy.tsx Update test to use shared CategoryGroup type.
apps/app/src/widgets/FilterPanel.tsx Switch CategoryGroup import to shared package.
apps/app/src/widgets/FilterPanel.cy.tsx Update test to use shared CategoryGroup type.
apps/app/src/utils/classification.tsx Remove duplicated isActivityFullyCategorized() implementation (now in shared).
apps/app/src/types.ts Remove duplicated category type definitions.
apps/app/src/popups/ShareMyContentModal.tsx Import isActivityFullyCategorized() from shared.
apps/app/src/paths/editor/EditorSettingsMode.tsx Import Category / CategoryGroup from shared.
apps/app/src/paths/editor/EditorSettingsMode.cy.tsx Update test imports to shared category types.
apps/app/src/paths/editor/EditorHeader.tsx Use shared types + shared isActivityFullyCategorized().
apps/app/src/paths/Explore.tsx Switch CategoryGroup import to shared package.
apps/app/src/drawers/ExploreFilterDrawer.tsx Switch CategoryGroup import to shared package.
apps/app/src/drawers/ExploreFilterDrawer.cy.tsx Update test import to shared CategoryGroup.
apps/api/src/types.ts Remove duplicated category type definitions.
apps/api/src/query/classification.ts Import CategoryGroup from shared for return typing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/shared/src/logic/browsable.ts Outdated
Comment thread packages/shared/src/logic/browsable.ts Outdated
cqnykamp and others added 2 commits April 23, 2026 04:05
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* Detect whether or not this activity has the required categories filled out.
* For each group that is required, make sure this activity has at least 1 category in that group.
*/
function isActivityFullyCategorized({
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isActivityFullyCategorized is implemented here but not exported, so it can’t actually be reused by other packages even though the PR description says this logic was moved to shared. Either export/re-export isActivityFullyCategorized (and/or rename isBrowsable to match the exported API), or update the shared API/PR description to reflect that only isBrowsable is intended to be public.

Suggested change
function isActivityFullyCategorized({
export function isActivityFullyCategorized({

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +19
export function isBrowsable(data: BrowsableData) {
const fullyCategorized = isActivityFullyCategorized(data);
// TODO: check for no errors
// TODO: check for no accessibility violations
return fullyCategorized;
}

/**
* Detect whether or not this activity has the required categories filled out.
* For each group that is required, make sure this activity has at least 1 category in that group.
*/
function isActivityFullyCategorized({
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isBrowsable currently just proxies the category-completeness check and has TODOs for additional criteria. The name implies broader semantics than it currently enforces, which can mislead callers and makes future implementation of the TODOs a potentially breaking behavioral change. Consider either renaming to something like isActivityFullyCategorized (and exporting that), or fully defining/implementing what “browsable” means in this shared helper.

Suggested change
export function isBrowsable(data: BrowsableData) {
const fullyCategorized = isActivityFullyCategorized(data);
// TODO: check for no errors
// TODO: check for no accessibility violations
return fullyCategorized;
}
/**
* Detect whether or not this activity has the required categories filled out.
* For each group that is required, make sure this activity has at least 1 category in that group.
*/
function isActivityFullyCategorized({
/**
* @deprecated This helper currently only checks category completeness.
* Prefer `isActivityFullyCategorized` until broader "browsable" criteria are
* fully defined and implemented.
*/
export function isBrowsable(data: BrowsableData) {
return isActivityFullyCategorized(data);
}
/**
* Detect whether or not this activity has the required categories filled out.
* For each group that is required, make sure this activity has at least 1 category in that group.
*/
export function isActivityFullyCategorized({

Copilot uses AI. Check for mistakes.
@cqnykamp cqnykamp closed this Apr 29, 2026
@cqnykamp cqnykamp deleted the refactor-move-browsable-validation branch April 29, 2026 17:33
Keep isBrowsable as the public interface, clarify its current behavior, and export the precise category helper for reuse.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants